faster randmtzig_exprnd (separate unlikely branch + inline) and rename it "randexp" #9144
Conversation
|
+1 for the rename. |
|
Also, I realise it's unexported, but it is used by Distributions, so a |
|
Just don't use the |
|
If you make this renaming, I'd suggest to backport it to 0.3. As @simonbyrne, Distributions.jl relies on this function. |
|
We can't change the 0.3 api. Best thing would be to deprecate, or handle it in distributions.jl with a version check. Thoughts? |
|
There's also Compat.jl. |
|
Compat.jl works. @simonbyrne @lindahua What would you guys prefer? @ivarne Would using compat.jl overcome the export issue? |
|
@rfourquet Could you also add the array fill versions for |
|
Compat.jl is fine. Or you may also let me know the exact version number where this changes. |
|
Ok - Compat.jl it is then. |
|
I have no "good" ideas for tests, but can at least add a test that the API works. I'm adding the array version, should we export those functions then? |
|
We just have to export |
rfourquet
force-pushed
the
rf/randexp
branch
from
ea612ee to
531338d
Compare
|
OK so I exported them, please check my wording in the documentation. I don't know about Compat.jl, must I do something? |
|
Compat.jl is an external package at https://github.com/JuliaLang/Compat.jl, so breaking changes can be made on base and Compat.jl will have to add some code to allow packages to work for both 0.3 and 0.4 by using the same syntax from Compat.jl. So if you're feeling particularly generous you can prepare a PR to Compat.jl that manages to work the same way both before and after this change. |
|
Basically, Distributions.jl uses the internal |
|
Thanks, so I understand that I just need to wait this to be merged to know the VERSION before issuing a PR to Compat.jl ( |
|
The last number in the version is the number of commits since last tagged version on the current branch. If you have the versions you say, that is a bug. Can you check it again? |
|
Thanks @ivarne, it's just that I had used a light tag for personal use, this is fixed if I remove it. |
faster randexp (separate unlikely branch + @inline) add array versions of randexp and export these functions
ViralBShah
force-pushed
the
rf/randexp
branch
from
531338d to
ccb1166
Compare
|
I squashed these and am merging. @rfourquet Could you issue the PR against Compat.jl? You should have commit access there, so you can just create a branch in Compat.jl. |
faster randmtzig_exprnd (separate unlikely branch + inline) and rename it "randexp"
|
@simonbyrne @lindahua Once this PR is merged, would you be able to update Distributions.jl? |
| randexp(MersenneTwister(10)) | ||
| randexp(MersenneTwister(10), 100000) | ||
| randexp!(MersenneTwister(10), Array(Float64, 100000)) | ||
|
|
There was a problem hiding this comment.
You should probably @test something too. This code will not verify anything other than that it doesn't throw exceptions. I know the results are random, but the size, type and range is not.
There was a problem hiding this comment.
Here I only duplicated what already existed for randn, but I agree that testing values should be added.
I personally find that it is good to test somewhere only the API (smaller sizes than 100000 would do it though). What we could do here is test that the global version and the version with a passed rng give the same results. I am planning to rewrite #8339 differently, with all API checks grouped together. Then it could be enough to do all the other tests on the global RNG.
add compatibility for randexp (Julia PR #9144)
Same transformation as was done recently on
randn.The rename was suggested by @ViralBShah. It turns out that this function is not exported by Base (nor documented it seems), should it be? If yes, I will write the array version
randexp!too.